-
Notifications
You must be signed in to change notification settings - Fork 63
feat: add json serde for REST Catalog request/response models #251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ca9db4e to
c27891e
Compare
5af1ab9 to
ae5190a
Compare
e393dde to
d3f9b81
Compare
d3f9b81 to
8444ef8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I mainly have two feedbacks other than those inline comments:
- All Java request and reposes classes inherit from
RESTMessagebelow with avalidatemethod. I think this is important because these objects may be created out of our control and must be validated before use.
/** Interface to mark both REST requests and responses. */
public interface RESTMessage {
/**
* Ensures that a constructed instance of a REST message is valid according to the REST spec.
*
* <p>This is needed when parsing data that comes from external sources and the object might have
* been constructed without all the required fields present.
*/
void validate();
}I'd propose to add a iceberg/catalog/rest/validator with interface like below for every request and response:
struct Validator {
static Status Validate(const ListNamespacesResponse& resp);
...
};- Interoperability is our top priority. I have followed all Java test cases for TableMetadata serde by adding them to
src/iceberg/test/metadata_serde_test.cc. There are a lot of test in the Java repo likeorg/apache/iceberg/rest/responses/TestListNamespacesResponse.javawhich contains both good and bad cases. It would be good to include them while implementing each objects.
4d34bf8 to
e19f740
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
This PR depends on and should be merged after #230. #249 #245 #248